feat: Replace deprecated Doppler recentLogs with Log Cache client#1237
feat: Replace deprecated Doppler recentLogs with Log Cache client#1237ZPascal wants to merge 19 commits intocloudfoundry:mainfrom
recentLogs with Log Cache client#1237Conversation
|
Thank you for the PR! Compilation is currently failing on the implementation missing the new recentLogs method that you added to the client |
anthonydahanne
force-pushed
the
issue-1181
branch
from
0cb4438 to
ce0cd68
Compare
|
Thank you @pivotal-david-osullivan & @anthonydahanne for fixing the tests and the reactor part. I am also working on the issue again from the SAP side and I plan to set up a BBL environment to test the change in a CF environment. Let's sync on Slack how we can handle it together and we can discuss the test setup/steps. |
cool! |
|
|
- The DopplerClient does not work with Loggregator >= 107.0.0, and we cannot obtain "recent" logs. Until this is fixed, we cannot cut a release. That is the only blocking test. - See cloudfoundrygh-1181, cloudfoundrygh-1230, cloudfoundrygh-1237
…CF 2.x line (#1265) * Restrict ApplicationsTests#logs to CF 2.x line - The DopplerClient does not work with Loggregator >= 107.0.0, and we cannot obtain "recent" logs. Until this is fixed, we cannot cut a release. That is the only blocking test. - See gh-1181, gh-1230, gh-1237 * Introduce Applications#logs API that will replace the doppler-based implementation Signed-off-by: Daniel Garnier-Moiroux <daniel.garnier-moiroux@broadcom.com> --------- Signed-off-by: Daniel Garnier-Moiroux <daniel.garnier-moiroux@broadcom.com>
ZPascal
force-pushed
the
issue-1181
branch
5 times, most recently
from
1edaf8c to
210376c
Compare
* because of the findFirst() on the envelopes, it could be type OUT or ERR, so we don't really care, as long as the payload is the same. Also, we don't care about the precise argument to the recentLogs call
The old "Applications.logs" method is keept for compatibility and when a log stream is needed. Adding new "logsRecent" method to access the logCache. integration-tests "ApplicationTest.logs" and "ApplicationTest.logsRecent" work. Minor changes in DefaultApplicationsTest in other JUnit tests to make them execute in Eclipse.
ZPascal
changed the title
recentLogs with Log Cache client
Hi @anthonydahanne, I've checked the possible options, and the implementation of the log streaming functionality should definitely be handled in a separate PR. It's out of the scope for the recent logs PR. |
Kehrlann
left a comment
Kehrlann
left a comment
There was a problem hiding this comment.
I think we should not introduce a new method in Applications, see comments in the PR directly.
...y-operations/src/main/java/org/cloudfoundry/operations/applications/DefaultApplications.java
Outdated
Show resolved
Hide resolved
cloudfoundry-client/src/main/java/org/cloudfoundry/logcache/v1/LogCacheClient.java
Outdated
Show resolved
Hide resolved
cloudfoundry-client/src/main/java/org/cloudfoundry/doppler/DopplerClient.java
Outdated
Show resolved
Hide resolved
| .map(EnvelopeBatch::getBatch) | ||
| .map(List::stream) | ||
| .flatMapIterable(envelopeStream -> envelopeStream.collect(Collectors.toList())) |
There was a problem hiding this comment.
| .map(EnvelopeBatch::getBatch) | |
| .map(List::stream) | |
| .flatMapIterable(envelopeStream -> envelopeStream.collect(Collectors.toList())) | |
| .flatMapIterable(EnvelopeBatch::getBatch) |
...y-operations/src/main/java/org/cloudfoundry/operations/applications/DefaultApplications.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * List the applications logs from logCacheClient. | ||
| * If no messages are available, an empty Flux is returned. | ||
| * | ||
| * @param request the application logs request | ||
| * @return the applications logs | ||
| */ | ||
| Flux<Log> logsRecent(ReadRequest request); |
There was a problem hiding this comment.
According to the original plan, we were to replace Flux<LogMessage> logs(LogsRequest request); by Flux<ApplicationLog> logs(ApplicationLogsRequest request);.
Maybe we should not introduce a new method, but update Flux<ApplicationLog> logs(ApplicationLogsRequest request); to use logcache. Thoughts?
There was a problem hiding this comment.
This would result in an incompatible change. I believe we should not release any incompatible changes in version 5.x.
There was a problem hiding this comment.
Just to clarify, my point is:
- Don't introduce a new method
- Change the implementation of
Flux<ApplicationLog> logs(ApplicationLogsRequest request);
This method was introduced specifically for this migration, see this commit: 1664cbf
It did not exist before 5.13.0 (release notes), where we said:
Deprecate doppler-based Applications#logs, prepare API types for the new Loggregator-based client
The idea was to make a method that uses Doppler, then change it to use Loggregator. That's why we introduced Doppler-agnostic types. The whole idea was to do a non-breaking change when swapping out Doppler ; the breaking change would be to remove Flux<LogMessage> logs(LogsRequest request); in v6.
(Also, in practice, this new method Flux<ApplicationLog> logs(ApplicationLogsRequest request); probably never worked for our users, because it's extremely likely they were already using a newer CF deployment version without Doppler when we released it)
There was a problem hiding this comment.
I understand your point, but it does feel quite opaque if we're making under-the-hood calls to another endpoint. I think this is something we should clearly document and communicate to the community so they're aware of the change.
...dry-operations/src/main/java/org/cloudfoundry/operations/_DefaultCloudFoundryOperations.java
Show resolved
Hide resolved
integration-test/src/test/java/org/cloudfoundry/operations/ApplicationsTest.java
Show resolved
Hide resolved
| Mono<String> applicationGuid = | ||
| getAppGuidFromAppName(cloudFoundryOperations, applicationName); |
There was a problem hiding this comment.
If we use Flux<ApplicationLog> logs(ApplicationLogsRequest request); for logs as mentioned further up in the comments, we can have a request that only cares about the application name, not its ID. Makes this test nicer.
There was a problem hiding this comment.
We shouldn’t release a breaking change right away. This change should be released in V6.
6 participants
Feat: Replace deprecated Doppler
recentLogswith Log Cache clientProblem
The
DopplerClient.recentLogs()endpoint relies on the Loggregator Traffic Controller/apps/{id}/recentlogsHTTP endpoint, which was removed in Loggregator ≥ 107.0 (shipped with CF Deployment ≥ 24.3 and Tanzu Application Service ≥ 4.0). Calls to this endpoint on modern CF foundations fail at runtime, and the related types (LogsRequest,LogMessagefromorg.cloudfoundry.doppler) caused compilation errors in the integration tests.Out of the scope
Fixes #1181 & #1230
Changes
cloudfoundry-clientDopplerClient.recentLogs()as@Deprecatedwith a Javadoc@deprecatednote pointing to the replacement API and documenting the platform version boundary (Loggregator < 107.0 / CFD < 24.3 / TAS < 4.0).LogCacheClient.recentLogs(ReadRequest)as the new, non-deprecated entry point.cloudfoundry-client-reactorReactorLogCacheEndpoints.recentLogs()— delegates to the existingread()method._ReactorLogCacheClient.cloudfoundry-operationsApplicationsinterface withlogsRecent(ReadRequest)backed byLogCacheClient.DefaultApplicationsto accept and hold aMono<LogCacheClient>alongside the existingMono<DopplerClient>.logs(LogsRequest)/logs(ApplicationLogsRequest)methods are now marked@Deprecated._DefaultCloudFoundryOperationsto exposelogCacheClientas an optional builder field (mirrors the existingdopplerClientpattern; missing client yields a descriptiveIllegalStateExceptionon first use).Integration & unit tests
AbstractOperationsTestandDefaultApplicationsTestto supply the newLogCacheClientmock.logsRecentDoppler,logsDoppler,logsNoAppDoppler) from Log Cache (logsRecentLogCache) paths; added@SuppressWarnings("deprecation")where the old API is still exercised.ApplicationsTest.logsRecent()integration test that provisions an application, fetches recent logs viaLogCacheClient, and asserts that each entry carries a validLogType(OUTorERR).IntegrationTestConfigurationto inject and wireLogCacheClientintoDefaultCloudFoundryOperations.Documentation (
README.md)## Deprecationssection (with a[!WARNING]callout) right after## Versions, covering:DopplerClient.recentLogs(),RecentLogsRequest,LogMessage),[!NOTE]callout in the### CloudFoundryClient, DopplerClient, UaaClient Builderssection cross-linking to the new deprecation notice.Migration guide
Checklist
logsRecentLogCache)logsRecent)@Deprecated+ Javadoc@deprecatedREADME.mdupdated with deprecation notice and migration guideIntegrationtest results
Logs of the Integration test run
The following command was used for the integration test execution:
Integration Tests
Log-Cache Integration Tests: